Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changement du type pour le Mandat #815

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

mdulac
Copy link
Contributor

@mdulac mdulac commented Oct 30, 2020

No description provided.

@jdauphant jdauphant temporarily deployed to aplus-demo-task-mandat--vqg4jj October 30, 2020 17:02 Inactive
Copy link
Collaborator

@niladic niladic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je pense que si tu veux avoir un modèle plus propre, il ne faut pas rajouter de la serialization dedans. J'avais créé serializers.DataModel dans cette optique, je pense que c'est la solution la plus simple à maintenir à long terme. Peut-être en renommant JsonDataModel et en créant PgDataModel avec les serializers anorm et des case class qui correspondent aux lignes (genre ApplicationRow) lorsque le modèle ne match plus la BDD.

Comment on lines +39 to +59
implicit val Reads: Reads[Answer] = Json
.reads[Answer]
.map(answer =>
answer.copy(creationDate = answer.creationDate.withZoneSameInstant(Time.timeZoneParis))
)

implicit val Writes: Writes[Answer] = Json.writes[Answer]

implicit val answerListParser: anorm.Column[List[Answer]] =
nonNull { (value, meta) =>
val MetaDataItem(qualified, _, _) = meta
value match {
case json: org.postgresql.util.PGobject =>
Json.parse(json.getValue).as[List[Answer]].asRight[Nothing]
case json: String => Json.parse(json).as[List[Answer]].asRight[Nothing]
case _ =>
TypeDoesNotMatch(
s"Cannot convert $value: ${className(value)} to List[Answer] for column $qualified"
).asLeft[List[Answer]]
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En fait avoir la logique de serialization dans le modèle ça n'a pas de sens. Ce n'est pas clair, ça mixe de la logique métier avec de la logique de serialization. Aussi juste la dépendance "modèle dépends des serializers anorm/json" ça sens pas bon.

)
}

final case class Mandat(_type: MandatType, date: String)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'aime pas trop le _ avant le nom, car ça signifie souvent "privé" ou "non utilisé, mais quand même utile" dans certains autres langages. Que penses-tu de type_ ? ou simplement `type` ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type_ c'est ok pour moi. A la base j'avais mis type, mais le parsing Twirl n'a pas l'air d'aimer :(

Comment on lines 196 to 244
implicit val Parser: RowParser[Application] =
(get[UUID]("id") ~
get[ZonedDateTime]("creation_date").map(_.withZoneSameInstant(Time.timeZoneParis)) ~
str(columnName = "creator_user_name") ~
get[UUID]("creator_user_id") ~
str(columnName = "subject") ~
str(columnName = "description") ~
get[Map[String, String]]("user_infos") ~
get[Map[UUID, String]]("invited_users") ~
get[UUID]("area") ~
bool(columnName = "irrelevant") ~
get[List[Answer]]("answers") ~
int("internal_id") ~
bool("closed") ~
get[List[UUID]]("seen_by_user_ids") ~
str("usefulness").? ~
get[ZonedDateTime]("closed_date").? ~
bool("expert_invited") ~
bool("has_selected_subject") ~
str("category").? ~
get[Map[String, Long]]("files") ~
Application.Mandat.Parser)
.map {
case id ~ creation ~ creatorName ~ creatorId ~ subject ~ description ~ userInfos ~ invitedUsers
~ area ~ irrelevant ~ answers ~ internalId ~ closed ~ seen ~ usefulness ~ closedDate ~ experts ~ hasSelectedSubject ~ category ~ files ~ mandat =>
Application(
id,
creation,
creatorName,
creatorId,
subject,
description,
userInfos,
invitedUsers,
area,
irrelevant,
answers,
internalId,
closed,
seen,
usefulness,
closedDate,
experts,
hasSelectedSubject,
category,
files,
mandat
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non mais simplement définis une ApplicationRow qui matche la ligne PG, et tu fais des méthodes de conversion Application <=> ApplicationRow, c'est tellement plus simple. Et les erreurs de consistence peuvent être loggués dans la transformation avec un log.warn puisqu'en théorie si ApplicationRow => Application fail, c'est un bug de notre part, ce n'est pas de la faute de l'utilisateur.

@@ -5,6 +5,7 @@ import java.time.{LocalDate, ZonedDateTime}
import java.util.UUID

import actions._
import cats.implicits.catsSyntaxOptionId
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import cats.implicits.catsSyntaxOptionId

@mdulac mdulac changed the title Application / Mandat model Refactoring Changement du type pour le Mandat Nov 2, 2020
@jdauphant jdauphant temporarily deployed to aplus-demo-task-mandat--vqg4jj November 2, 2020 19:00 Inactive
@jdauphant jdauphant temporarily deployed to aplus-demo-task-mandat--vqg4jj November 3, 2020 13:59 Inactive
@jdauphant jdauphant temporarily deployed to aplus-demo-task-mandat--vqg4jj November 3, 2020 14:51 Inactive
@@ -5,7 +5,7 @@ import java.time.{LocalDate, ZonedDateTime}
import java.util.UUID

import actions._
import cats.implicits.catsSyntaxTuple2Semigroupal
import cats.implicits.{catsSyntaxOptionId, catsSyntaxTuple2Semigroupal}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import cats.implicits.{catsSyntaxOptionId, catsSyntaxTuple2Semigroupal}

tu es sûr que c'est nécessaire ?

Comment on lines +36 to +61

object Answer {

implicit val Reads: Reads[Answer] = Json
.reads[Answer]
.map(answer =>
answer.copy(creationDate = answer.creationDate.withZoneSameInstant(Time.timeZoneParis))
)

implicit val Writes: Writes[Answer] = Json.writes[Answer]

implicit val answerListParser: anorm.Column[List[Answer]] =
nonNull { (value, meta) =>
val MetaDataItem(qualified, _, _) = meta
value match {
case json: org.postgresql.util.PGobject =>
Json.parse(json.getValue).as[List[Answer]].asRight[Nothing]
case json: String => Json.parse(json).as[List[Answer]].asRight[Nothing]
case _ =>
TypeDoesNotMatch(
s"Cannot convert $value: ${className(value)} to List[Answer] for column $qualified"
).asLeft[List[Answer]]
}
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Du coup ces modifs ne sont plus nécessaires ?

Comment on lines +18 to +20
implicit val MandatTypeParser: Column[Option[MandatType]] =
implicitly[Column[Option[String]]]
.map(_.flatMap(dataModelDeserialization))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non utilisé ?

hasSelectedSubject: Boolean = false,
category: Option[String] = Option.empty[String],
files: Map[String, Long] = Map.empty[String, Long],
mandatType: Option[Application.Mandat.MandatType],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour postgres c'est un varchar, que penses-tu de mettre la conversion dans toApplication plutôt ?

Suggested change
mandatType: Option[Application.Mandat.MandatType],
mandatType: Option[String],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants